-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes AgentRandom uniform int #411
Conversation
Probably worth aiming to be consistent with what users would expect in terms of inclusivity.
In terms of floating point:
So curand seems to differ from what standard libraries suggest. Would it not be better to modify |
re: floating point. |
I would assume that the cost of an extra floating point subtraction is going to be negligible compared to the cost of actually generating a random number, but yeah benching it is never a bad idea. Could also expand the testing of RNG (#330) while i'm blocking changes to CUDASimulation. |
I'm not really sure how #330 should look. I certainly don't think it will fit inside the regular test suite. |
No, but we are documenting the inclusivity of the ranges for float and int variants, so atleast verifying that would be good. |
The int generation works now, small histogram suggests its got no huge bias, and I can't see it being changed further (besides lopping off the inclusive upper bound), so that doesn't seem like an issue. |
Int generation seems to be inclusive most places so i'd keep it inclusive. Ranges should certainly be tested though (as changing what uniform returns would potentailyl lead to out of bounds etc) and worth having a test. |
I want to make some more changes to this before it's merged (mostly to how double uniform is calculated) |
Actually can't do what I thought I could, so won't bother. |
Windows/Debug/SEATBELTS=ON Python tests pass (I wanted to check RTC would build the updated header)
|
This is technically a breaking change, however only so much that epsilon is shifted. The cost of random generation is therefore marginally higher, due to additional computation.
This is technically a breaking change, however only so much that epsilon is shifted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look OK at a glance + your testing mentioned on slack sounds OK. Worth getting merged in during 2.0.0-rcX given its potentially a significant change for users.
Still needs more thorough testing eventually so we'll keep #330 open.
Tested 600 ints (from the same thread) in the range [0, 5]
Got this histogram, which seems as you would expect.
curand rng returns in the range
(0, 1]
, so the math is a bit arkwards.Furthermore, as we return an inclusive range, we must add an additional 1 to the range (
max-min
).Perhaps there's a slight argument for the upper bound to be exclusive to simplify the maths.